fix(ivy): instantiate dirs in correct order #23178

kara · 2018-04-05T04:59:03Z

Currently, directives have to be listed in directive defs in the correct order or injected directives will not be found. For example, consider the following setup, where DirA injects DirB, but DirA is created first.

<div dirA dirB></div>

class DirA {
  constructor(dirB: DirB) {...}
}

class DirB {...}

In this case,DirB won't be found by DI because it hasn't been created yet.

This PR allows DI to eagerly instantiate a directive dependency if we know it's been defined on the same node, but not yet created. The extra step will only occur on first template pass because directives are now saved to tView.directives in the correct instantiation order.

It also adds logic to throw a helpful error if there's a circular dependency.

Notes:

currentMatches has been added to the TView as a temporary holder for selector matches on the node while injection occurs. This allows DI to check whether a directive is queued to be created on the same node and if so, use the def provided to instantiate the injected directive first.
- Unfortunately, we can't cut out the middle man and check directly if the current node would match the selector in directiveInject because it would require an expensive megamorphic property access to get the selector (token.ngDirectiveDef.selector).
- Another option might be to pass the matches into the factory fn directly, but this would muddy up the API and add extra bytes to the generated code.

mhevery · 2018-04-05T17:12:34Z

packages/core/src/render3/di.ts

+      // If we *didn't* find the directive for the token and we are searching the current node's
+      // injector, it's possible the directive is on this node and hasn't been created yet.
+      let instance: T|null;
+      if (injector === di && (instance = searchMatchesQueuedForCreation<T>(node, token))) {


Does this information get cached so that subsequent instantiations are fast? In other words, can we rearrange the order of directives until it is the correct order?

It looks to me that every time we want to inject parent directive we will go to searchMatchesQueuedForCreation which will have negative perf impact. I think the logic should be something like

if (first instantiation) { record the order in which the directives have been created (lookahead if needed) update the current list of directives to the recorded list. } else { directives are in correct order just instantiate them. }

As I think you mentioned in a later comment, this is the way it already works. The directives are saved in the correct order after the first template pass, so subsequent runs will just instantiate the directives in the right order.

It's worth noting that if you are injecting a parent directive, this search flow will never be entered (even in the first template pass) because DI would have retrieved the parent bloom injector and the injector === di check would fail.

The only time the search occurs is:
- on first template pass
- if token's bit has been added to the bottom level bloom filter during directive matching (so we know it's in this list)
- AND the directive was not already created (out of order)

mhevery · 2018-04-05T17:14:38Z

packages/core/src/render3/instructions.ts

+ * If we visit a directive that has a value set to CIRCULAR, we know we've
+ * already seen it, and thus have a circular dependency.
+ */
+export const CIRCULAR = '__CIRCULAR__';


because it is a string someone could create it. Better to do {mark: 'circular'} This way no one can got hold of that instance.

The matches array only contains directive defs, directive indices, and this constant, so there's no way user-provided values could be in there. I ended up using a string to avoid unnecessary object creation. Happy to change if you still think it's worth it.

mhevery · 2018-04-05T17:16:45Z

packages/core/src/render3/instructions.ts

-    node.tNode = tData[index] = tNode;
+    node.tNode = tData[index] = createTNode(name, attrs || null, containerData);
+    cacheMatchingDirectivesForNode(node.tNode, currentView.tView, localRefs || null);
+  } else {


Ohh I see, I think this is where the caching happens. So my previous comment may not be applicable.

Yeah, directives are saved in the correct order for instantiation after the first run.

buildsize · 2018-04-05T17:50:43Z

File name	Previous Size	New Size	Change
bundle.min.js	52.21 KB	52.64 KB	438 bytes (1%)
bundle.min.js.brotli	14.06 KB	14.14 KB	79 bytes (1%)

mary-poppins · 2018-04-05T18:10:20Z

You can preview 235612e at https://pr23178-235612e.ngbuilds.io/.

trotyl · 2018-04-05T18:14:40Z

packages/core/test/render3/di_spec.ts

+      /** <div dirA dirB dirC></div> */
+      const App = createComponent('app', function(ctx: any, cm: boolean) {
+        if (cm) {
+          elementStart(0, 'div', ['dirA', '', 'dirB', '', 'dirC', 'dirC']);


'dirC', 'dirC' -> 'dirC', ''?

Thanks! It shouldn't affect the test, but it doesn't match the comment, so I've updated the PR.

mary-poppins · 2018-04-05T18:30:58Z

You can preview e855190 at https://pr23178-e855190.ngbuilds.io/.

kara · 2018-04-05T20:39:57Z

presubmit

angular-automatic-lock-bot · 2019-09-13T12:21:57Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Apr 5, 2018

kara added the state: WIP label Apr 5, 2018

kara force-pushed the order branch from 5ec1cc0 to a77a1ce Compare April 5, 2018 05:03

kara added target: major This PR is targeted for the next major release comp: ivy labels Apr 5, 2018

kara force-pushed the order branch 3 times, most recently from 40bd9f8 to 7b1ec24 Compare April 5, 2018 16:56

kara added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Apr 5, 2018

kara requested a review from mhevery April 5, 2018 16:58

mhevery approved these changes Apr 5, 2018

View reviewed changes

fix(ivy): instantiate dirs in correct order

235612e

kara force-pushed the order branch from 7b1ec24 to 235612e Compare April 5, 2018 17:36

angular deleted a comment from mary-poppins Apr 5, 2018

kara added type: bug/fix and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 5, 2018

trotyl reviewed Apr 5, 2018

View reviewed changes

fixup! fix(ivy): instantiate dirs in correct order

e855190

kara added the action: merge The PR is ready for merge by the caretaker label Apr 5, 2018

IgorMinar closed this in 628303d Apr 5, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ivy): instantiate dirs in correct order #23178

fix(ivy): instantiate dirs in correct order #23178

kara commented Apr 5, 2018 •

edited

mhevery Apr 5, 2018

kara Apr 5, 2018 •

edited

mhevery Apr 5, 2018

kara Apr 5, 2018 •

edited

mhevery Apr 5, 2018

kara Apr 5, 2018

buildsize bot commented Apr 5, 2018 •

edited

mary-poppins commented Apr 5, 2018

trotyl Apr 5, 2018

kara Apr 5, 2018

mary-poppins commented Apr 5, 2018

kara commented Apr 5, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

fix(ivy): instantiate dirs in correct order #23178

fix(ivy): instantiate dirs in correct order #23178

Conversation

kara commented Apr 5, 2018 • edited

mhevery Apr 5, 2018

Choose a reason for hiding this comment

kara Apr 5, 2018 • edited

Choose a reason for hiding this comment

mhevery Apr 5, 2018

Choose a reason for hiding this comment

kara Apr 5, 2018 • edited

Choose a reason for hiding this comment

mhevery Apr 5, 2018

Choose a reason for hiding this comment

kara Apr 5, 2018

Choose a reason for hiding this comment

buildsize bot commented Apr 5, 2018 • edited

mary-poppins commented Apr 5, 2018

trotyl Apr 5, 2018

Choose a reason for hiding this comment

kara Apr 5, 2018

Choose a reason for hiding this comment

mary-poppins commented Apr 5, 2018

kara commented Apr 5, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

kara commented Apr 5, 2018 •

edited

kara Apr 5, 2018 •

edited

kara Apr 5, 2018 •

edited

buildsize bot commented Apr 5, 2018 •

edited